Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intermediate stage of reading heat flux values from ASCOT5 HDF5 file #8

Merged
merged 23 commits into from
Aug 12, 2021

Conversation

bielsnohr
Copy link
Collaborator

Summary

A number of methods have been added to interrogate an HDF5 file produced by an ASCOT5 run and extract the particle energies and the mesh elements that the particles have hit. These are important steps towards getting heat flux values on the tokamak mesh. All functionality has been added in the AscotProblem class. These will be called from AscotProblem::syncSolutions(), and most of the framework is there now.

Objective

C++ is not my primary language, and I have limited experience with the OO paradigm. It would be good to get some comments around these aspects. In particular:

  1. In a somewhat functional programming style, I use quantities returned by methods for further operations. Good example is getActiveEndState() where I am returning an HDF5 Group object which has been created within the method. Naively, there must be overhead for the copy operation since this object won't have scope outside of the method, but from my reading online, lots of compilers handle this this days through Return Value Optimisation (RVO). Some thoughts about whether this is good design would be appreciated.
  2. The HDF5 C++ API isn't the most intuitive. Have I made things more complicated than they need to be? Is a shift to the C API advisable?
  3. Should I use a try construct when opening the HDF5 file?

This needs to be done in both the main app Makefile and the unit test
app Makefile, for some reason. I would have expected MOOSE to pick up
flags for app that have been included into another.
This is an intermediate step in writing this unit test for
getWallTileHits()
The ASCOT5 HDF5 file is correctly queried for the walltile dataset, and
expected values are returned according to read_walltile unit test.
Helper routine calculateRelativisticEnergy started and unit test needs
to be filled out.
@bielsnohr bielsnohr linked an issue Jul 29, 2021 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@dynamicist dynamicist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answers to specific questions:

"In a somewhat functional programming style, I use quantities returned by methods for further operations. Good example is getActiveEndState() where I am returning an HDF5 Group object which has been created within the method. Naively, there must be overhead for the copy operation since this object won't have scope outside of the method, but from my reading online, lots of compilers handle this this days through Return Value Optimisation (RVO). Some thoughts about whether this is good design would be appreciated."

  • Even if RVO/NRVO doesn't happen std::move will be called (from C++11 onwards at least I think, with 1 possible exception prior to C++20) which will avoid the temporary copy
  • You could explicitly call std::move on the return value, however std::move will disable NRVO if the compiler implements it. Pretty much all compiles will optimise the code to use std::move anyway, so there's no benefit to relying on the compiler optimization here, which will use RVO if it exists, and std::move if it doesn't.
  • The only exception is if you return an rvalue reference and you are using a language standard prior to C++20, in which case it isn't eligible for NRVO and you will need to call std::move explicitly
  • In terms of good design, it looks fine to me (there's a reason there are lots of compiler optimisations for it!) and is commonly done.

"The HDF5 C++ API isn't the most intuitive. Have I made things more complicated than they need to be? Is a shift to the C API advisable?"

  • If you shift to the C API you'd have to write your own wrapper for the HDF5 C functions, so probably the best approach is to use the existing C++ API (which is a just a wrapper to the C API anyway) unless there's a good reason not to. The code doesn't seem overly complicated to me, and looking at the C-C++ API table I think the C++ wrapper is more intuitive from an OO mindset than the C API.

"Should I use a try construct when opening the HDF5 file?"

  • the HDF5 C++ API seems to say that these functions can throw exceptions, so yes you should have a try...catch block to catch them... Unless you are working under the assumption that the caller of your library will deal with any exceptions... which may be the case here as some of the other functions throw MooseExceptions? So maybe catch the exception from the HDF5 api and raise a MooseException instead.

And some general comments:

  • I'd avoid using .C for c++ files just in case anyone ever wants to use the code on windows (which is case insensitive for filenames) or with a compiler other than GNU C++. *.cpp is the most portable extension, followed by .cxx and .cc (and a lot of people will use .hpp for c++ headers to indicate the language, but that won't bother the compilers).
  • I think getActiveEndstate, getWallTileHits, getParticleEnergies and calculateRelativisticEnergy can all be static functions at present. However, I'm not sure if that's the best design - maybe it would be better if the H5file was a class member variable rather than passed as an argument to the functions. Similarly with the ascot5_active_endstate Group variable.
  • Also, are the getActiveEndstate, getWallTileHits, getParticleEnergies functions intended to be called from outside the class (other than in testing)? If not then they should be private... or alternatively you can have another class that reads HDF5 files with more general versions of these functions as public members that read data from the HDF5 file, and test those, rather than expose them as public functions in the AscotProblem class for the purposes of testing only.

Hopefully that all makes sense!

include/problems/AscotProblem.h Outdated Show resolved Hide resolved
src/problems/AscotProblem.C Outdated Show resolved Hide resolved
src/problems/AscotProblem.C Outdated Show resolved Hide resolved
include/problems/AscotProblem.h Outdated Show resolved Hide resolved
include/problems/AscotProblem.h Outdated Show resolved Hide resolved
include/problems/AscotProblem.h Outdated Show resolved Hide resolved
include/problems/AscotProblem.h Outdated Show resolved Hide resolved
Following discovery of way to get ASCOT5 to compile on Ubuntu 20.04, it
is now possible to use Helen's moose-ubuntu image instead of getting
things to run on 18.04. This has the added advantage that the HDF5
libraries are now also more up-to-date and my unit tests pass without
modification.
This also resulted in catching the fact that I had inadvertently updated
the submodule to a commit too far ahead of the ascot5.2 tag, meaning it
did not run with the C program. Cherry picked the relevant commits back
to the ascot5.2 tag as a stop gap.
@bielsnohr
Copy link
Collaborator Author

And some general comments:

* I'd avoid using .C for c++ files just in case anyone ever wants to use the code on windows (which is case insensitive for filenames) or with a compiler other than GNU C++.  *.cpp is the most portable extension, followed by .cxx and .cc (and a lot of people will use .hpp for c++ headers to indicate the language, but that won't bother the compilers).

I completely agree, but unfortunately this what the MOOSE framework uses. I am hesitant to change to .cpp in case it breaks things.

* I think getActiveEndstate, getWallTileHits, getParticleEnergies and calculateRelativisticEnergy can all be static functions at present. However, I'm not sure if that's the best design - maybe it would be better if the H5file was a class member variable rather than passed as an argument to the functions. Similarly with the ascot5_active_endstate Group variable.

This will be resolved by suggestion below about moving these routines into a separate class.

* Also, are the getActiveEndstate, getWallTileHits, getParticleEnergies functions intended to be called from outside the class (other than in testing)? If not then they should be private... or alternatively you can have another class that reads HDF5 files with more general versions of these functions as public members that read data from the HDF5 file, and test those, rather than expose them as public functions in the AscotProblem class for the purposes of testing only.

Agreed.

@bielsnohr bielsnohr merged commit cda252a into main Aug 12, 2021
@bielsnohr bielsnohr deleted the 2-add-read-heat-flux-hdf5 branch August 12, 2021 16:24
@bielsnohr bielsnohr restored the 2-add-read-heat-flux-hdf5 branch August 12, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to read heat flux values from the HDF5 format that ASCOT5 uses
2 participants